Skip to content

Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change.#11659

Merged
auto-submit[bot] merged 2 commits intoflutter:mainfrom
aam:interface-address-dart-sdk-breaking-change
May 7, 2026
Merged

Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change.#11659
auto-submit[bot] merged 2 commits intoflutter:mainfrom
aam:interface-address-dart-sdk-breaking-change

Conversation

@aam
Copy link
Copy Markdown
Member

@aam aam commented May 6, 2026

This addresses dart sdk breaking change dart-lang/sdk#63216.

flutter/flutter#186155 tracks undoing this once dart rolls into flutter.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 6, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the multicast_dns tests to use InterfaceAddress instead of InternetAddress within the FakeNetworkInterface implementation. Feedback identifies a compilation error in the FakeInterfaceAddress class where the address property returns a String instead of an InternetAddress. Additionally, the reviewer noted that several methods were included that are not part of the InterfaceAddress interface and suggested returning null for the broadcast property to improve stability.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/multicast_dns/test/client_test.dart (286-320)

high

The FakeInterfaceAddress implementation has a significant type error and includes methods not present in the InterfaceAddress interface:

  1. Type Error: InterfaceAddress.address must return an InternetAddress object, but your implementation returns a String (from _internetAddress.address). This will cause a compilation error.
  2. Extra Methods: isLinkLocal, isLoopback, isMulticast, and reverse() are members of InternetAddress, not InterfaceAddress. They should be removed to strictly adhere to the interface.
  3. Broadcast: Returning null for the broadcast property is generally safer than throwing an UnimplementedError if the property is accessed during interface enumeration.
class FakeInterfaceAddress implements InterfaceAddress {
  const FakeInterfaceAddress(this.address);

  @override
  final InternetAddress address;

  @override
  String get host => address.host;

  @override
  Uint8List get rawAddress => address.rawAddress;

  @override
  InternetAddressType get type => address.type;

  @override
  int get prefixLength => 0;

  @override
  InternetAddress? get broadcast => null;
}

@aam
Copy link
Copy Markdown
Member Author

aam commented May 6, 2026

@stuartmorgan-g you might know - how do you deal with dart sdk breaking changes that affect this repo?

This repo uses dart sdk from flutter, landing new dart sdk into flutter is impossible because the roll breaks these flutter packages, but then updating fluttter packages and rolling updated flutter packages together with roll of dart sdk is impossible because flutter packages uses old dart sdk(from flutter) so updating PR(like this one) fails.

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

how do you deal with dart sdk breaking changes that affect this repo?

Usually we don't; I'm having a hard time remembering a case where the Dart SDK broke us with a change that wasn't in a major Dart version.

There are a few problems, which have different solutions:

  1. The flutter tool actually uses this package, so there's a true circular dependency. This is bad, and I'm in the process of standing up flutter/core-packages precisely to fix issues like that (this package is slated to be moved as soon as I have the CI set up there). Luckily we can ignore this problem here since this is test-only.
  2. The analyzer run in flutter/flutter blocking the Dart->flutter/flutter roll can be addressed by temporarily making that test in flutter/flutter non-blocking to temporarily break that artificial cycle in our test infra. However, that will just kick the can down the road to:
  3. Once the roll lands in flutter/flutter, the flutter/flutter->flutter/packages roll will be blocked, because we analyze our packages on both stable and main.

We'll need a solution to 3, so if we can solve it in a way that fixes 2 we can avoid having to turn tests off and on in flutter/flutter.

I haven't dug into the exact details here; could we do something slightly gross with dynamic to make this compile for both versions? If not, given the limited scope in this particular instance I think my preference would be to just comment out this test, with a tracking issue to turn it back on once this breaking change reaches stable.

@github-actions github-actions Bot removed the CICD Run CI/CD label May 6, 2026
@aam
Copy link
Copy Markdown
Member Author

aam commented May 6, 2026

I haven't dug into the exact details here; could we do something slightly gross with dynamic to make this compile for both versions? If not, given the limited scope in this particular instance I think my preference would be to just comment out this test, with a tracking issue to turn it back on once this breaking change reaches stable.

thank you Stuart. I commented out the test with actual changes as comments so this can be uncommented and test restored as soon as dart sdk lands in flutter.

@aam aam changed the title Update FakeNetworkInterface to accommodate dart sdk breaking change. Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change. May 6, 2026
@aam aam added the CICD Run CI/CD label May 6, 2026
@iinozemtsev iinozemtsev added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2026
@auto-submit auto-submit Bot merged commit 0411f1d into flutter:main May 7, 2026
84 checks passed
@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

Too late for this PR obviously, but in the future if something like this is necessary please put the issue link directly in the code as a comment so that it's trivial for someone to map from the weird commented out code to the tracking issue without having to dig through git blame.

so this can be uncommented and test restored as soon as dart sdk lands in flutter.

Again, it's not as soon as it lands, it's when it reaches the stable channel of Flutter. For something landing in main right now, we expect that to be in August. The file will be in this state for months, not a day or two.

@aam
Copy link
Copy Markdown
Member Author

aam commented May 7, 2026

Again, it's not as soon as it lands, it's when it reaches the stable channel of Flutter. For something landing in main right now, we expect that to be in August. The file will be in this state for months, not a day or two.

I missed that. What is the reason to wait for stable?

because we analyze our packages on both stable and main.

Sounds like a pretty limiting requirement. Is that because there is only one channel no branches of flutter packages available?

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

stuartmorgan-g commented May 7, 2026

What is the reason to wait for stable?

I don't think I'm following; the next line of your comment is a quote that would be my answer, which makes me think I'm not understanding what the question actually is here.

because we analyze our packages on both stable and main.

Sounds like a pretty limiting requirement.

In practice I consider it to have been extremely successful in accomplishing the desired goal (ensuring that fixes can get out quickly to the customers using a stable version of Flutter rather than main, which is pretty close to all of them) with minimal effort.

It has also had the unintended but useful secondary effect of making it more obvious to people working in flutter/flutter why we generally want to avoid hard breaking changes in favor of phased migrations. So to the extent that this has been limiting in practice, I think it's mostly been a positive rather than a negative.

Is that because there is only one channel no branches of flutter packages available?

If I'm understanding this question correctly, yes, this policy would not be necessary if we maintained release branches and a branch management process with cherry picks for all of our packages.

So far, in the years we've had this policy, I haven't seen evidence that the eng cost of that overhead would give nearly enough value to be worthwhile.

@aam
Copy link
Copy Markdown
Member Author

aam commented May 7, 2026

What is the reason to wait for stable?

I don't think I'm following; the next line of your comment is a quote that would be my answer, which makes me think I'm not understanding what the question actually is here.

Basically, I wanted to confirm my understanding. Besides running analysis(which is what was quoted) I think you also actually run the tests both on stable and on main ensuring that that like you said that flutter/packages is fully compatible with stable.

because we analyze our packages on both stable and main.

Sounds like a pretty limiting requirement.

In practice I consider it to have been extremely successful in accomplishing the desired goal (ensuring that fixes can get out quickly to the customers using a stable version of Flutter rather than main, which is pretty close to all of them) with minimal effort.
It has also had the unintended but useful secondary effect of making it more obvious to people working in flutter/flutter why we generally want to avoid hard breaking changes in favor of phased migrations. So to the extent that this has been limiting in practice, I think it's mostly been a positive rather than a negative.

Is that because there is only one channel no branches of flutter packages available?

If I'm understanding this question correctly, yes, this policy would not be necessary if we maintained release branches and a branch management process with cherry picks for all of our packages.
So far, in the years we've had this policy, I haven't seen evidence that the eng cost of that overhead would give nearly enough value to be worthwhile.

By "limiting" I meant that you are not able to use features in dart/flutter until they are released in stable creating this many-months feedback delay. I think it would have been valuable to get that flutter/packages team's feedback sooner.

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

By "limiting" I meant that you are not able to use features in dart/flutter until they are released in stable

The alternatives are:

  1. we constantly maintain two branches of every package, where one branch supports stable, and we cherry-pick essentially everything between branches, including maintaining diverging copies where the new features cause code differences, or
  2. features and bug fixes to packages are routinely delayed for multiple months in getting to clients.

As I said above, I haven't seen any evidence that (1) would be worth the effort. (2) seems like a significant net loss to me.

creating this many-months feedback delay. I think it would have been valuable to get that flutter/packages team's feedback sooner.

Could you point me to some examples of where this has been as issue? I'm not sure why landing changes on main would be necessary for trying something out and providing feedback.

okorohelijah pushed a commit to alex-medinsh/flutter that referenced this pull request May 7, 2026
…r#186191)

flutter/packages@67ec0d3...0411f1d

2026-05-07 aam@google.com Comment out the test that uses implemented
NetworkInterface to accommodate dart sdk breaking change.
(flutter/packages#11659)
2026-05-06 fondoger@outlook.com [vector_graphics_compiler] Fix HSL color
parsing for decimal percentage components (flutter/packages#11619)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@aam
Copy link
Copy Markdown
Member Author

aam commented May 7, 2026

Could you point me to some examples of where this has been as issue? I'm not sure why landing changes on main would be necessary for trying something out and providing feedback.

Isn't this introduction of InterfaceAddress class with its support for broadcast address be something that flutter/packages are not able to use until several months in the future?

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

Sorry, I'm not following. What valuable feedback would landing 948cc4a have provided that is being delayed?

@aam
Copy link
Copy Markdown
Member Author

aam commented May 7, 2026

Sorry, I'm not following. What valuable feedback would landing 948cc4a have provided that is being delayed?

I'm thinking of a potential feedback where some flutter/packages's use of InterfaceAddress revealed some crashes or other unexpected behaviors resulting from original change that would require fixes in dart sdk.

It's what I think that normally is described as "eating your own dog food" kind of thing.

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

I'm thinking of a potential feedback where some flutter/packages's use of InterfaceAddress revealed some crashes or other unexpected behaviors resulting from original change that would require fixes in dart sdk.

Anyone is welcome to open a draft PR with those changes as soon as it rolls through main to this repo (usually a day or two delay from flutter/flutter); our presubmit and postsubmit tests are pretty much identical, so just putting up the PR should give all of that information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD p: multicast_dns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants